Fix HTTP/Stdio Transport UX and Test Bug #530
Conversation
…ault to Color.white
Tested everything, works like a charm
We know what it's managing
We also update the namespace for ManageVFX
Scripts for running the NL/T/GO test suites locally against a GUI Unity Editor, complementing the CI workflows in .github/workflows/. Benefits: - 10-100x faster than CI (no Docker startup) - Real-time Unity console debugging - Single test execution for rapid iteration - Auto-detects HTTP vs stdio transport Usage: ./scripts/local-test/setup.sh # One-time setup ./scripts/local-test/quick-test.sh NL-0 # Run single test ./scripts/local-test/run-nl-suite-local.sh # Full suite See scripts/local-test/README.md for details. Also updated .gitignore to: - Allow scripts/local-test/ to be tracked - Ignore generated artifacts (reports/*.xml, .claude/local/, .unity-mcp/)
Move SaveDirtyScenesIfNeeded() call outside the PlayMode conditional so EditMode tests don't get blocked by Unity's "Save Scene" modal dialog. This prevents MCP from timing out when running EditMode tests with unsaved scene changes.
…plementation Merged EditorStateV2 into EditorState, making get_editor_state the canonical resource. Updated Unity C# to use EditorStateCache directly. Enhanced Python implementation with advice/staleness enrichment, external changes detection, and instance ID inference. Removed duplicate EditorStateV2 resource and legacy fallback mapping.
Added strongly-typed Pydantic models for EditorStateV2 schema in Python and corresponding C# classes with JsonProperty attributes. Updated C# to serialize using typed classes instead of anonymous objects. Python now validates the editor state payload before returning it, catching schema mismatches early.
…ation Merged run_tests_async into run_tests, making async job-based execution the default behavior. Removed synchronous blocking test execution. Updated RunTests.cs to start test jobs immediately and return job_id for polling. Changed TestJobManager methods to internal visibility. Updated README to reflect single run_tests_async tool. Python implementation now uses async job pattern exclusively.
…ure/local-test-harness
It should reduce conflicts with other Unity MCPs that users try, and to comply with Unity's requests regarding use of their company and product name
We now differentiate whether it's HTTP local or remote
The FAST_FAIL_TIMEOUT class attribute was referenced on line 149 but never defined, causing AttributeError on every ping attempt. This error was silently caught by the broad 'except Exception' handler, causing all fast-fail commands (read_console, get_editor_state, ping) to fail after 6 seconds of retries with 'ping not answered' error. Added FAST_FAIL_TIMEOUT = 10 to define a 10-second timeout for fast-fail commands, matching the intent of the existing fast-fail infrastructure.
… and Quaternion
Dry-run validation now validates value formats, not just property existence:
- AnimationCurve: Validates structure ({keys:[...]} or direct array), checks
each keyframe is an object, validates numeric fields (time, value, inSlope,
outSlope, inWeight, outWeight) and integer fields (weightedMode)
- Quaternion: Validates array length (3 for Euler, 4 for raw) or object
structure ({x,y,z,w} or {euler:[x,y,z]}), ensures all components are numeric
Refactored shared validation helpers into appropriate locations:
- ParamCoercion: IsNumericToken, ValidateNumericField, ValidateIntegerField
- VectorParsing: ValidateAnimationCurveFormat, ValidateQuaternionFormat
Added comprehensive XML documentation clarifying keyframe field defaults
(all default to 0 except as noted).
Added 5 new dry-run validation tests covering valid and invalid formats
for both AnimationCurve and Quaternion properties.
Resolved conflict in plugin_hub.py: both branches added FAST_FAIL_TIMEOUT (fixing the same missing constant bug). Kept msanatan's version (2.0s with better comment) as it's more appropriate for fast-fail behavior.
- test_refresh_unity_retry_recovery: Mock now handles both refresh_unity and get_editor_state commands (refresh_unity internally calls get_editor_state when wait_for_ready=True) - test_run_tests_async_forwards_params: Mock response now includes required 'mode' field for RunTestsStartResponse Pydantic validation - test_get_test_job_forwards_job_id: Updated to handle GetTestJobResponse as Pydantic model instead of dict (use model_dump() for assertions)
Follow-up to PR CoplayDev#527: Since SaveDirtyScenesIfNeeded() now runs for all test modes, update the warning message to say 'tests' instead of 'PlayMode tests'.
…p detection When polling for test completion, MCP clients like Cursor can detect the repeated get_test_job calls as 'looping' and terminate the agent. Added wait_timeout parameter that makes the server wait internally for tests to complete (polling Unity every 2s) before returning. This dramatically reduces client-side tool calls from 10-20 down to 1-2, avoiding loop detection. Usage: get_test_job(job_id='xxx', wait_timeout=30) - Returns immediately if tests complete within timeout - Returns current status if timeout expires (client can call again) - Recommended: 30-60 seconds
Root cause: WriteToConfigTests.TearDown() was unconditionally deleting UseHttpTransport EditorPref even when tests were skipped on Windows (NUnit runs TearDown even after Assert.Ignore). Changes: - Fix WriteToConfigTests to save/restore prefs instead of deleting - Add centralized ShouldForceUvxRefresh() for local dev path detection - Clean stale Python build/ artifacts before client configuration - Improve reload handler flag management to prevent stuck Resuming state - Show Resuming status during stdio bridge restart - Initialize client config display on window open - Add DevModeForceServerRefresh to EditorPrefs window known types
📝 WalkthroughWalkthroughThis PR centralizes dev-mode refresh detection and local server artifact cleanup across multiple editor services and helpers. Previously scattered DevModeForceServerRefresh EditorPrefs checks are consolidated into new AssetPathUtility helpers. Related services now delegate to these centralized methods instead of local logic. Additionally, stdio/HTTP resume handling after domain reloads is refined for improved reliability. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
409-410: Fix comment style and verify cleanup timing.Line 409 uses
///(XML documentation comment style) inside a method body. Use//instead for inline comments.The cleanup timing is correct—artifacts are removed before server startup, preventing stale cache issues.
✨ Fix comment style
- /// Clean stale Python build artifacts when using a local dev server path + // Clean stale Python build artifacts when using a local dev server path AssetPathUtility.CleanLocalServerBuildArtifacts();
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/Editor/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Services/StdioBridgeReloadHandler.csMCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.csMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.csTestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-12-29T15:23:11.613Z
Learnt from: msanatan
Repo: CoplayDev/unity-mcp PR: 491
File: MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs:78-115
Timestamp: 2025-12-29T15:23:11.613Z
Learning: In MCPForUnity, prefer relying on the established testing process to catch UI initialization issues instead of adding defensive null checks for UI elements in editor windows. This means during reviews, verify that tests cover UI initialization paths and that code avoids repetitive null-check boilerplate in Editor Windows. If a UI element can be null, ensure there is a well-tested fallback or that its initialization is guaranteed by design, rather than sprinkling null checks throughout editor code.
Applied to files:
MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.csMCPForUnity/Editor/Clients/McpClientConfiguratorBase.csMCPForUnity/Editor/Services/ClientConfigurationService.csMCPForUnity/Editor/Helpers/AssetPathUtility.csMCPForUnity/Editor/Helpers/ConfigJsonBuilder.csMCPForUnity/Editor/Helpers/CodexConfigHelper.csMCPForUnity/Editor/Services/ServerManagementService.csMCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.csMCPForUnity/Editor/Services/StdioBridgeReloadHandler.csMCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs
📚 Learning: 2025-09-03T16:00:55.839Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 0
File: :0-0
Timestamp: 2025-09-03T16:00:55.839Z
Learning: ComponentResolver in UnityMcpBridge/Editor/Tools/ManageGameObject.cs is a nested static class within ManageGameObject, not a sibling type. The `using static MCPForUnity.Editor.Tools.ManageGameObject;` import is required to access ComponentResolver methods directly without the outer class qualifier.
Applied to files:
MCPForUnity/Editor/Services/ClientConfigurationService.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to mitigate argument parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path with spaces.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.cs
📚 Learning: 2025-09-05T16:22:04.960Z
Learnt from: dsarno
Repo: CoplayDev/unity-mcp PR: 265
File: README.md:204-204
Timestamp: 2025-09-05T16:22:04.960Z
Learning: In the Unity MCP project, the ServerInstaller.cs creates a symlink from ~/Library/AppSupport to ~/Library/Application Support on macOS to avoid spaces in paths that can cause arg parsing and quoting issues in some MCP clients. The README documentation should use the shortened AppSupport path, not the full "Application Support" path.
Applied to files:
MCPForUnity/Editor/Helpers/AssetPathUtility.cs
🧬 Code graph analysis (6)
MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
AssetPathUtility(15-294)IsLocalServerPath(209-218)CleanLocalServerBuildArtifacts(245-269)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (2)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)MCPForUnity/Editor/Helpers/McpLog.cs (3)
McpLog(7-52)Info(37-41)Warn(43-46)
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (3)
AssetPathUtility(15-294)CleanLocalServerBuildArtifacts(245-269)ShouldForceUvxRefresh(194-204)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)
MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs (4)
MCPForUnity/Editor/Services/MCPServiceLocator.cs (1)
MCPServiceLocator(11-91)MCPForUnity/Editor/Services/Transport/TransportManager.cs (3)
TransportManager(11-148)TransportManager(20-25)IsRunning(132-132)MCPForUnity/Editor/Services/BridgeControlService.cs (1)
TransportMode(25-30)MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (1)
MCPForUnity/Editor/Constants/EditorPrefKeys.cs (1)
EditorPrefKeys(7-56)
🔇 Additional comments (23)
MCPForUnity/Editor/Windows/EditorPrefs/EditorPrefsWindow.cs (1)
42-42: LGTM! Properly exposes the DevModeForceServerRefresh preference.The addition correctly registers the
DevModeForceServerRefreshEditorPref key as a boolean type in the EditorPrefs management window. This allows developers to view and modify the centralized refresh flag introduced in this PR.MCPForUnity/Editor/Windows/Components/ClientConfig/McpClientConfigSection.cs (1)
87-91: LGTM! Proper initialization of client display on window open.The addition correctly initializes the UI state for the first selected client when the window opens, mirroring the callback behavior triggered by dropdown changes. The bounds checking in each method safely handles edge cases like empty client lists.
MCPForUnity/Editor/Helpers/AssetPathUtility.cs (4)
189-204: LGTM: Well-designed centralized refresh detection.The auto-enable behavior for local paths is a good developer experience improvement that prevents stale cache issues during local development.
206-218: LGTM: Correct local path detection.The logic properly handles both
file://protocol and absolute filesystem paths across platforms.
220-237: LGTM: Clean local path extraction.The
Substring(7)operation is safe because theStartsWithcheck at Line 230 guarantees sufficient length.
239-269: Verify the recursive deletion doesn't impact active processes.The recursive deletion of the
build/directory is aggressive but justified for removing stale Python artifacts. Ensure that this cleanup happens before any server process starts to avoid file-in-use errors.Looking at the usage in ServerManagementService.cs (Line 410) and the PR description, cleanup occurs before server startup, which is the correct timing. Consider documenting this timing requirement in the method's XML documentation to prevent future misuse.
MCPForUnity/Editor/Clients/McpClientConfiguratorBase.cs (2)
470-472: LGTM: Correct centralized refresh check.The refactoring correctly delegates to
AssetPathUtility.ShouldForceUvxRefresh()and maintains the same UVX flag behavior.
589-590: LGTM: Consistent centralized refresh check.The refactoring is consistent with the Register method and correctly uses the centralized helper.
MCPForUnity/Editor/Services/ServerManagementService.cs (1)
1242-1243: LGTM: Correct centralized refresh check.The refactoring correctly uses the centralized helper for UVX cache management.
MCPForUnity/Editor/Helpers/CodexConfigHelper.cs (3)
20-27: LGTM: Clean refactoring to centralized refresh check.The signature change is safe (private method) and the centralized approach improves maintainability.
55-55: LGTM: Call site correctly updated.The call to
AddDevModeArgscorrectly matches the new signature.
204-204: LGTM: Call site correctly updated.The call to
AddDevModeArgscorrectly matches the new signature.MCPForUnity/Editor/Helpers/ConfigJsonBuilder.cs (2)
54-56: Verify the null client behavior is intentional.The refactored transport logic adds a defensive check for client HTTP support. When
clientis null, the expressionclient?.SupportsHttpTransport != falseevaluates totrue, defaulting to HTTP support. Confirm this is the intended behavior for backward compatibility.The change prevents attempting HTTP transport when a client explicitly doesn't support it, which is a good defensive improvement.
154-178: LGTM: Correct refactoring to centralized refresh check.The signature change is safe (private method) and the centralized approach is consistent with changes across other files.
TestProjects/UnityMCPTests/Assets/Tests/EditMode/Helpers/WriteToConfigTests.cs (2)
24-37: Excellent fix for the test harness bug.Saving the original EditorPrefs values before
Assert.Ignore()is the correct solution, since NUnit executesTearDown()even when tests are skipped. The defensive comment on line 33 clearly documents this non-obvious behavior.
77-87: Proper restoration of user settings in TearDown.The conditional restore/delete logic correctly handles both scenarios: restoring original values when keys existed, and deleting keys when they didn't. This prevents the stdio transport flicker described in the PR objectives.
MCPForUnity/Editor/Services/ClientConfigurationService.cs (1)
26-31: Good preventive cleanup for local server artifacts.The centralized cleanup via
AssetPathUtility.CleanLocalServerBuildArtifacts()prevents stale build artifacts from causing cache issues when using a local server path. The placement before both individual and bulk configuration operations ensures consistent behavior.Also applies to: 38-42
MCPForUnity/Editor/Services/StdioBridgeReloadHandler.cs (3)
30-32: Clearer runtime state detection.The explicit split into
tmRunningandhostRunningimproves readability and aligns with the comment explaining why both checks are necessary (CI starts via StdioBridgeHost bypassing TransportManager).
64-72: Immediate flag clearing prevents stuck "Resuming..." state.This change addresses the core UX issue described in the PR objectives. By clearing the flag immediately when not resuming, the UI won't get stuck displaying "Resuming..." after conditions change.
93-98: Defensive flag cleanup after start attempt.Clearing the resume flag in the continuation (regardless of start success/failure) provides additional protection against stuck state. The detailed comments explaining threading safety and the fresh-flag-on-next-reload guarantee are helpful.
MCPForUnity/Editor/Windows/Components/Connection/McpConnectionSection.cs (3)
158-161: Proactive cleanup of stale resume flags.Clearing both stdio and HTTP resume flags when the user manually changes transport ensures that automatic resumption logic doesn't conflict with explicit user intent.
281-283: More reliable state detection after domain reload.Using EditorPrefs as the source of truth instead of the dropdown value is a good reliability improvement, since the dropdown might not be fully initialized immediately after a domain reload.
336-367: "Resuming..." state improves stdio transport UX.The detection and display of the resuming state addresses the PR objective to "show 'Resuming' status during stdio bridge restart." The implementation correctly:
- Detects the resuming condition via the
ResumeStdioAfterReloadflag- Provides clear visual feedback ("Resuming...")
- Disables interactive controls (toggle button, port field) during the transient state
- Re-enables the toggle button when the session becomes active (line 336)
上游變更 (v7.0.0 → v9.0.3)： - GameObject 工具集重構 (CoplayDev#518) - VFX 管理功能 (CoplayDev#520) - 批次執行錯誤處理改進 (CoplayDev#531) - Windows 長路徑問題修復 (CoplayDev#534) - HTTP/Stdio 傳輸 UX 改進 (CoplayDev#530) - LLM 工具註解功能 (CoplayDev#480) 衝突解決： - modify/delete：接受刪除（架構已重構） - content：接受 v9.0.3 版本（2020 相容性修正將另行處理）
2 participants
Summary
Root cause:
WriteToConfigTests.TearDown()was unconditionally deleting theUseHttpTransportEditorPref even when tests were skipped on Windows (NUnit runs TearDown even afterAssert.Ignore()). This caused the transport mode to unexpectedly reset.Stdio Transport UX Improvements
ShouldForceUvxRefresh()for local dev path detectionbuild/artifacts before client configuration (prevents cache issues)Changes
Test Bug Fix
Minor Additions
DevModeForceServerRefreshto EditorPrefs window known typesFiles Changed
WriteToConfigTests.csStdioBridgeReloadHandler.csMcpConnectionSection.csMcpClientConfigSection.csMcpClientConfiguratorBase.csServerManagementService.csAssetPathUtility.csShouldForceUvxRefresh()helperConfigJsonBuilder.csCodexConfigHelper.csClientConfigurationService.csEditorPrefsWindow.csTesting
Summary by CodeRabbit
Release Notes
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.